feat(#1381): Add a way to specify "inject-only" with @JacksonInject #5175

giulong · 2025-05-24T11:23:43Z

useInput=TRUE in JacksonInject now make deserialization discard the injected value in favor of input (if any)

…lization discard the injected value in favor of input (if any)

giulong · 2025-05-24T11:24:32Z

src/test/java/com/fasterxml/jackson/databind/deser/inject/JacksonInject2678Test.java

@@ -1,4 +1,4 @@
-package com.fasterxml.jackson.databind.tofix;
+package com.fasterxml.jackson.databind.deser.inject;


moved since this should not fail anymore

giulong · 2025-05-24T11:33:36Z

src/test/java/com/fasterxml/jackson/databind/deser/inject/JacksonInject1381Test.java

+    }
+
+    @Test
+    @DisplayName("input YES, injectable YES, useInput DEFAULT|FALSE => injected")


According to useInput's javadocs:

Default is OptBoolean.DEFAULT, which translates to OptBoolean.TRUE: this is
for backwards compatibility (2.8 and earlier always allow binding input value).

This combination: input YES, injectable YES, useInput DEFAULT should actually behave as if we had useInput = TRUE, that is dropping the injected value and returning the input.

Nevertheless, since useInput never worked that way, this would mean introducing breaking changes. My suggestion is to keep things as per this test and change the javadoc accordingly:

Default is OptBoolean.DEFAULT, which translates to OptBoolean.FALSE

giulong · 2025-05-24T11:42:22Z

...test/java/com/fasterxml/jackson/databind/deser/inject/JacksonInject1381WithOptionalTest.java

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class JacksonInject1381WithOptionalTest extends DatabindTestUtil {


This class shows how the behavior changes for all the combinations of the other test class when all injected fields are optional. Do you think we still need a required property in the annotation?

I feel like combining useInput and optional would cover all cases, also because optional should drive the same logic (but reversed) of required. Given this, adding a required property would make things unclear for the users, and the internal logic would be quite messy. Does this make sense?

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java

src/test/java/com/fasterxml/jackson/databind/deser/inject/JacksonInject1381Test.java

cowtowncoder

LGTM.

cowtowncoder · 2025-06-05T04:08:48Z

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java

+
+                // 04-Jun-2025, tatu: [databind#1381]: default for "useInput" is false
+                if (!Boolean.TRUE.equals(useInput)) {
+                    builder.addInjectable(PropertyName.construct(m.getName()),


This is part I don't fully understand: if injectable is only added if no input is to be used, how do things work with useInput = false, injectable value being present.

As per this test, with no input but injected value present, the injected value is always returned regardless of useInput:

@Test @DisplayName("input NO, injectable YES, useInput DEFAULT|TRUE|FALSE => injected") void test2() throws Exception { assertEquals("injected", injectedMapper.readValue(empty, InputDefault.class).getField()); assertEquals("injected", injectedMapper.readValue(empty, InputTrue.class).getField()); assertEquals("injected", injectedMapper.readValue(empty, InputFalse.class).getField()); }

I read it like "we have no input but we have a value to inject, so we don't care about anything else: let's use the injected value". Is this the expected behavior? Or you meant something else?

Logic-wise, yes, you are correct (expected behavior I agree with).

But I meant code; meaning... what does addInjectable() do -- to me, it's the thing that injects non-input (injectable) value. So why is that contingent on "useInput"? And I think check here seems wrong in that sense: injectable value may be needed even input is used.
It is not only injected if useInput is false (or missing); can also be injected if useInput is true but input has no value for property.

It's a way to eagerly discard the injectable if we're going to use the input anyways. I think it's better in terms of performance since any further evaluation is useless, and there are many methods involved down that path.
But I also get this:

injectable value may be needed even input is used

Not now maybe, but in general I get that if a field is marked as injected, we should add it to the internal injectables for whatever future logic.

I can try to change this, which means passing useInput to the builder (last param in the snippet), as we made for optional:

builder.addInjectable(PropertyName.construct(m.getName()), m.getType(), beanDesc.getClassAnnotations(), m, entry.getKey(), optional, useInput);

Maybe we could also think of passing the whole injectableValue to the builder, instead of passing its properties one by one.

But I think this is not going to be easy. What do you think, is it worth it?

Yes, figured it was meant as optimization.
But the problem with " if we're going to use the input anyways" is that there might not be any input.
So it's not "use input if any matching, otherwise null" but rather "use input if present; if not, use injected value". Otherwise why even mark it with @JacksonInject at all, actually.

So we cannot know statically which one to use.

And yes, I think this is necessary to handle properly.

But I understand things are messy, tricky, complicated, esp. when passing things via Constructors (CreatorProperty properties) vs Field/MethodProperty ones.

cowtowncoder

Actually, no: if I change things to put @JacksonInject into Constructor parameter (instead of Field) -- common alternative (although adding on Field is allowed too), a few tests fail.

So this doesn't quite work fully yet.

cowtowncoder · 2025-06-05T04:17:38Z

Ok. I don't think this fix is complete: I changed test set up slightly (in a way I think users are more likely to use it -- although original way is legit too... but both should work) and now a few tests fail.

So would need to figure a way to make things fully work with combination of input and/or injectable values.

giulong · 2025-06-07T15:35:40Z

Fair point, I missed that constructor properties take another path. On it

…e useInput, optional, and/or the DeserializationFeature

…operties as well

giulong · 2025-06-15T18:43:41Z

src/main/java/com/fasterxml/jackson/databind/deser/impl/PropertyValueBuffer.java

@@ -218,6 +219,25 @@ public Object[] getParameters(SettableBeanProperty[] props)
        if (_anyParamSetter != null) {
            _creatorParameters[_anyParamSetter.getParameterIndex()] = _createAndSetAnySetterValue();
        }
+
+        for (int ix = 0; ix < props.length; ++ix) {


This is the moment when creator properties are resolved, so I think it's ok to have the logic here, as per the method's javadoc as well:

/**
* Method called to do necessary post-processing such as injection of values
* and verification of values for required properties,
* after either {@link #assignParameter(SettableBeanProperty, Object)}
* returns true (to indicate all creator properties are found), or when
* then whole JSON Object has been processed,
*/
public Object[] getParameters(SettableBeanProperty[] props)

I placed the new part right before the code that checks for null creator properties in case we have an injected value to use, so to not fail. Should I move it after?

giulong · 2025-06-15T18:56:37Z

We now have the tests to cover all the scenarios (I guess), meaning combinations of injected creator properties vs fields, with or without JacksonInject.optional() and DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE. So, in case other changes are needed, at least we have a no regression suite to check them.

cowtowncoder · 2025-06-16T02:08:43Z

We now have the tests to cover all the scenarios (I guess), meaning combinations of injected creator properties vs fields, with or without JacksonInject.optional() and DeserializationFeature.FAIL_ON_UNKNOWN_INJECT_VALUE. So, in case other changes are needed, at least we have a no regression suite to check them.

I will try to have a good look tomorrow, to answer with proper understanding.

Thank you for going through this again.

…Inject to the injectables

giulong · 2025-06-20T16:00:49Z

I reworked the logic to always add the properties annotated with @JacksonInject to the injectables, regardless of their optional and useInput values.

Few things to note, for completeness:

I'm doing comparisons like value == JacksonInject.Value.empty(). I think it's better to not use equals here, since we're looking for that specific constant.

In this case:

@Test
@DisplayName("FAIL_ON_UNKNOWN_INJECT_VALUE NO, input NO, injectable NO, useInput DEFAULT|TRUE|FALSE => exception")
void test1() {
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputDefault.class));
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputDefaultConstructor.class));
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputTrue.class));
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputTrueConstructor.class));
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputFalse.class));
    assertThrows(ValueInstantiationException.class,
            () -> plainMapper.readValue(empty, InputFalseConstructor.class));
}

we expect an exception, since we have no input nor injectable value. Since FAIL_ON_UNKNOWN_INJECT_VALUE is disabled as well, the injection process does not throw the usual MissingInjectableValueExcepion. The ValueInstantiationException comes from the AnnotatedConstructor.call method, since the arg provided is actually JacksonInject.Value.empty().
In the injection process, we use that value as a way to signal that we tried to resolve the injectable, but due to useInput, optional and FAIL_ON_UNKNOWN_INJECT_VALUE the injection should actually be skipped. I think this is fine since we need a special marker (null isn't good since it could be a valid value), but from the user's perspective I also think throwing a generic ValueInstantiationException is not the best: seeing a message like Cannot construct instance of ..., problem: argument type mismatch could be misleading.

This implementation worsens If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed twice. #4218, making the injection process happen thrice. Checked with JacksonInject4218Test.

I hope the overall implementation is good enough to be merged, despite the bullets above. What do you think?

feat(FasterXML#1381): useInput=TRUE in JacksonInject now make deseria…

9a42cca

…lization discard the injected value in favor of input (if any)

giulong commented May 24, 2025

View reviewed changes

cowtowncoder reviewed Jun 5, 2025

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerFactory.java Outdated Show resolved Hide resolved

Merge branch '2.x' into feature/databind-1381-inject-only

d714aeb

cowtowncoder reviewed Jun 5, 2025

View reviewed changes

src/test/java/com/fasterxml/jackson/databind/deser/inject/JacksonInject1381Test.java Outdated Show resolved Hide resolved

cowtowncoder added 2 commits June 4, 2025 20:51

Change exception throws

d6bee5f

...

bc094d8

cowtowncoder approved these changes Jun 5, 2025

View reviewed changes

cowtowncoder reviewed Jun 5, 2025

View reviewed changes

cowtowncoder requested changes Jun 5, 2025

View reviewed changes

Add release notes; broke tests by changing annotation placements

a12a3d7

giulong added 4 commits June 7, 2025 17:53

test: minor refactor to apply coding conventions

69c603e

test(FasterXML#1381): covering all cases for JacksonInject, whether w…

191292c

…e useInput, optional, and/or the DeserializationFeature

feat(FasterXML#1381): JacksonInject.useInput works for constructor pr…

5705aff

…operties as well

Merge branch '2.x' into feature/databind-1381-inject-only

bc9d37e

giulong commented Jun 15, 2025

View reviewed changes

giulong added 3 commits June 20, 2025 14:50

Merge branch 'FasterXML:2.x' into feature/databind-1381-inject-only

272c499

test(FasterXML#1381): minor code cleaning removing unused getters

5da46ac

feat(FasterXML#1381): always adding properties annotated with Jackson…

deb1741

…Inject to the injectables

giulong requested a review from cowtowncoder June 20, 2025 16:02

JooHyukKim mentioned this pull request Jul 1, 2025

If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed twice. #4218

Open

1 task

		@@ -1,4 +1,4 @@
		package com.fasterxml.jackson.databind.tofix;
		package com.fasterxml.jackson.databind.deser.inject;

Uh oh!

feat(#1381): Add a way to specify "inject-only" with @JacksonInject #5175

Are you sure you want to change the base?

feat(#1381): Add a way to specify "inject-only" with @JacksonInject #5175

Uh oh!

Conversation

giulong commented May 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cowtowncoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giulong Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowtowncoder left a comment

Choose a reason for hiding this comment

Uh oh!

cowtowncoder commented Jun 5, 2025

Uh oh!

giulong commented Jun 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giulong commented Jun 15, 2025

Uh oh!

cowtowncoder commented Jun 16, 2025

Uh oh!

giulong commented Jun 20, 2025

Uh oh!

Uh oh!

giulong Jun 15, 2025 •

edited

Loading